fix: Bulk email recipient options by Inferato · Pull Request #171 · openedx/frontend-app-communications

Inferato · 2023-11-09T20:24:25Z

Description

According to the chapter Sending Email Messages to Learners in Different Enrollment Tracks course can may more than one mode and instructor must have the ability to send the email for users in this modes.
Communication MFE had a hardcoded 2 modes for every course. Sometimes it lead to the error.

courseModes was added to courseMetadata to display real course email recipient options.

Related MR:

openedx-webhooks · 2023-11-09T20:24:29Z

Thanks for the pull request, @Inferato! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

codecov · 2023-11-09T22:27:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (56e1781) 83.01% compared to head (59a02eb) 83.18%.
Report is 8 commits behind head on master.

❗ Current head 59a02eb differs from pull request most recent head ea5d6a2. Consider uploading reports for the commit ea5d6a2 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
+ Coverage   83.01%   83.18%   +0.17%     
==========================================
  Files          46       47       +1     
  Lines         683      690       +7     
  Branches      132      135       +3     
==========================================
+ Hits          567      574       +7     
  Misses        116      116

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

peterkulko · 2023-11-13T11:17:08Z

+          courseModes
+          && courseModes.length > 1
+          && courseModes.map((courseMode) => (


I propose to move logical operations into a separate variable, this will simplify the readability of the code.

Suggested change

courseModes

&& courseModes.length > 1

&& courseModes.map((courseMode) => (

const isCourseModes = courseModes && courseModes.length > 1;

...

isCourseModes && courseModes.map((courseMode) => (

Good point! I already moved it. Thx!

peterkulko · 2023-11-13T11:18:54Z

@@ -0,0 +1,18 @@
+import { Factory } from 'rosie'; // eslint-disable-line import/no-extraneous-dependencies
+
+const courseModeFactory = () => {


It would be a good idea to add some jsDocs documentation for this code. What do you think?

OK. It has already been added. Check please and add any suggestions if needed.

peterkulko · 2023-11-13T11:19:56Z

    <BulkEmailProvider>
-      <BulkEmailForm courseId="test" cohorts={cohorts} />
+      <BulkEmailForm
+        courseId="test"


Just a suggestion: let's come up with a more meaningful name for this identifier 💯

Do you have any examples? I used the same name as in the backend.

mockCourseId or exampleCourseId
Please, If it's possible

Ok, if it is necessary. This ID was here before I made the changes to the tests so I didn't check this part for correctness

I decided to not change courseId value to save the common style of test values.

peterkulko · 2023-11-16T11:05:18Z

+ *
+ * @returns {Object[]} An array of course mode objects generated by different factories.
+ */
+import { Factory } from 'rosie'; // eslint-disable-line import/no-extraneous-dependencies


It seems to me that there is no need to write jsDocs for import.

This is a doc for the module, not for import

I would remove it, but I leave the decision up to you 👍

arbrandes

Approved in principle, though I have a nit with a variable name. Thanks!

Let's wait to merge this until after the backend PR merges, though.

arbrandes · 2023-11-20T15:38:30Z

+    additionalCohorts,
+    courseModes,
+  } = props;
+  const isCourseModes = courseModes && courseModes.length > 1;


Suggested change

const isCourseModes = courseModes && courseModes.length > 1;

const hasCourseModes = courseModes && courseModes.length > 1;

arbrandes · 2023-11-20T15:38:46Z

-        </Form.Checkbox>
+        {
+          // additional modes
+          isCourseModes


Suggested change

isCourseModes

hasCourseModes

dyudyunov · 2024-02-06T13:40:01Z

Sorry for the delays, Taras is no longer a member of RG.
Currently, I'm gathering all the PRs he worked on. I will check their statuses and update them if necessary.

I will leave the status update comments on each PR's comments ASAP

dyudyunov · 2024-02-07T12:36:10Z

Hello @arbrandes

Could you close this PR? I can't add edits to the source branch so I opened a new one where I took the source branch from here as it is and just applied your suggestions.

mphilbrick211 · 2024-02-21T15:30:44Z

closing per the comment above.

openedx-webhooks · 2024-02-21T15:30:47Z

@Inferato Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 9, 2023

fix: Course mode is used to show the correct bulk email options

02d17ae

Inferato force-pushed the lytvynenko/RGOeX-26199/bulk_email_checkbox_master branch from 9cb6fa8 to 02d17ae Compare November 9, 2023 22:24

This was referenced Nov 9, 2023

fix: Course mode is used to show the correct bulk email options #172

Closed

fix: course mode added to the metadata openedx/openedx-platform#33690

Merged

fix: course mode added to the metadata openedx/openedx-platform#33691

Merged

peterkulko reviewed Nov 13, 2023

View reviewed changes

fix: Logical operations moved into a separate variable

59a02eb

peterkulko reviewed Nov 16, 2023

View reviewed changes

peterkulko approved these changes Nov 16, 2023

View reviewed changes

docs: Unnecessary comment deleted

ea5d6a2

arbrandes requested changes Nov 20, 2023

View reviewed changes

mphilbrick211 closed this Feb 21, 2024

		@@ -0,0 +1,18 @@
		import { Factory } from 'rosie'; // eslint-disable-line import/no-extraneous-dependencies

		const courseModeFactory = () => {

	const isCourseModes = courseModes && courseModes.length > 1;
	const hasCourseModes = courseModes && courseModes.length > 1;

Uh oh!

Conversation

Inferato commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

openedx-webhooks commented Nov 9, 2023

Uh oh!

codecov Bot commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterkulko Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Inferato Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arbrandes left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dyudyunov commented Feb 6, 2024

Uh oh!

dyudyunov commented Feb 7, 2024

Uh oh!

mphilbrick211 commented Feb 21, 2024

Uh oh!

openedx-webhooks commented Feb 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Inferato commented Nov 9, 2023 •

edited

Loading

codecov Bot commented Nov 9, 2023 •

edited

Loading

peterkulko Nov 16, 2023 •

edited

Loading

Inferato Nov 16, 2023 •

edited

Loading

arbrandes left a comment •

edited

Loading